fix(3468):Tool update resets visibility to public when field not provided by KKNithin · Pull Request #3503 · IBM/mcp-context-forge

KKNithin · 2026-03-05T14:47:25Z

🔗 Related Issue

Closes #3468

📝 Summary

Added logic to handle visibility when not provided

🏷️ Type of Change

✅ Checklist

Code formatted (make black isort pre-commit)
Tests added/updated for changes
Documentation updated (if applicable)
No secrets or credentials committed

jonpspri · 2026-03-05T19:53:22Z

Note — additional fix bundled in this PR:

Besides the visibility-reset bug (#3468), commit 4bf8dcbf9 also fixes an auth_type wipe bug: previously, when tool_update.auth was None (i.e. the caller didn't provide auth fields), the else branch set tool.auth_type = None, silently erasing existing authentication credentials on the tool.

The fix removes that else branch so omitting auth in an update payload now correctly preserves the existing auth_type and auth_value. A dedicated test (test_update_tool_auth_not_reset_when_omitted) covers this.

Signed-off-by: Nithin Katta <Nithin.Katta@ibm.com>

…ate schema Signed-off-by: Nithin Katta <Nithin.Katta@ibm.com>

…ck_tool fixture Signed-off-by: Nithin Katta <Nithin.Katta@ibm.com>

…k visibility conflicts - Change ToolUpdate.visibility default from "public" to None so omitting the field preserves the existing value instead of resetting to public. - Remove client-settable team_id/owner_email from ToolUpdate; derive ownership fields from the DB record for conflict checks. - Extract _check_tool_name_conflict() to handle public/team/private scoped uniqueness checks in a single reusable method. - Add conflict checking when visibility changes without a name change. - Fix custom_name_ref fallback to use existing custom_name when it diverges from name (instead of incorrectly using the new name). - Remove the else branch that wiped auth_type when auth was not provided. Closes IBM#3468 Signed-off-by: Jonathan Springer <jps@s390x.com>

…complete conflict checks - Change ToolUpdate.visibility from Optional[str] to Optional[Literal["private", "team", "public"]] so Pydantic rejects invalid values at the schema boundary. - Add a warning log in _check_tool_name_conflict when a team/private visibility is missing the required team_id/owner_email, making skipped conflict checks observable. Signed-off-by: Jonathan Springer <jps@s390x.com>

crivetimihai · 2026-03-07T00:50:54Z

Review Summary

Rebased onto main — clean, no conflicts. Reviewed code, ran unit tests (all 307 pass), verified 100% differential test coverage, and performed live end-to-end testing against localhost:8080 (Docker Compose with PostgreSQL, nginx, 3 gateway replicas).

Findings

Correctness — All logic is sound:

Schema fix: ToolUpdate.visibility changed from default="public" to None with Literal validation.
Conflict check refactored into _check_tool_name_conflict() — handles public/team/private scopes correctly.
custom_name_ref resolution correctly handles diverged custom names.
Combined name+visibility changes use the target visibility for conflict checking.
Auth preservation: removed the else: tool.auth_type = None bug that wiped credentials.

Security — Improves security:

Derives team_id/owner_email from DB records, not client input (CLAUDE.md invariant).
Literal type constraint rejects invalid visibility values at schema boundary (HTTP 422).
The old code's tool_update.team_id reference was a latent crash bug (field doesn't exist on ToolUpdate).

Consistency — Aligns with resource_service.py and prompt_service.py patterns. The Literal constraint is stricter than those services (which use Optional[str]) — an improvement.

Test Coverage — 100% differential coverage. 14 new tests + helper covering all new code paths.

Performance — No additional DB queries introduced.

Live E2E Test Results

Test	Result
Create private tool, update desc without visibility → stays private	PASS
Update public tool desc without visibility → stays public	PASS
Invalid visibility value `"invalid_value"` → HTTP 422	PASS
Auth (`bearer`) preserved after update without auth field	PASS
Cross-scope rename (private → same name as public tool) → allowed	PASS
Same-scope name conflict (public → existing public name) → rejected	PASS
Admin UI: edit form pre-selects correct visibility radio	PASS
Admin UI: save description change → visibility preserved	PASS

LGTM.

crivetimihai

Approved — rebased, reviewed, unit-tested (307 pass), and E2E-verified against live deployment. All visibility preservation, conflict detection, auth preservation, and schema validation scenarios confirmed working.

…rror ToolNameConflictError formatted private-scope conflicts as "Public" because the else branch caught both public and private visibility. This path is newly reachable since the PR adds private conflict detection. Add an explicit "private" branch and a test. Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>

…ided (#3503) * fix(#3468): handle visibility in tool update conflict checks Signed-off-by: Nithin Katta <Nithin.Katta@ibm.com> * fix(#3468): remove default value from visibility field in tool update schema Signed-off-by: Nithin Katta <Nithin.Katta@ibm.com> * test(#3468): update tool visibility conflict check tests to use mock_tool fixture Signed-off-by: Nithin Katta <Nithin.Katta@ibm.com> * fix(#3468): harden tool update to preserve omitted fields and check visibility conflicts - Change ToolUpdate.visibility default from "public" to None so omitting the field preserves the existing value instead of resetting to public. - Remove client-settable team_id/owner_email from ToolUpdate; derive ownership fields from the DB record for conflict checks. - Extract _check_tool_name_conflict() to handle public/team/private scoped uniqueness checks in a single reusable method. - Add conflict checking when visibility changes without a name change. - Fix custom_name_ref fallback to use existing custom_name when it diverges from name (instead of incorrectly using the new name). - Remove the else branch that wiped auth_type when auth was not provided. Closes #3468 Signed-off-by: Jonathan Springer <jps@s390x.com> * fix(#3468): add visibility value validation and warning log for incomplete conflict checks - Change ToolUpdate.visibility from Optional[str] to Optional[Literal["private", "team", "public"]] so Pydantic rejects invalid values at the schema boundary. - Add a warning log in _check_tool_name_conflict when a team/private visibility is missing the required team_id/owner_email, making skipped conflict checks observable. Signed-off-by: Jonathan Springer <jps@s390x.com> * fix(#3468): label private conflicts correctly in ToolNameConflictError ToolNameConflictError formatted private-scope conflicts as "Public" because the else branch caught both public and private visibility. This path is newly reachable since the PR adds private conflict detection. Add an explicit "private" branch and a test. Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> --------- Signed-off-by: Nithin Katta <Nithin.Katta@ibm.com> Signed-off-by: Jonathan Springer <jps@s390x.com> Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> Co-authored-by: Nithin Katta <Nithin.Katta@ibm.com> Co-authored-by: Jonathan Springer <jps@s390x.com> Co-authored-by: Mihai Criveti <crivetimihai@gmail.com>

…ided (#3503) * fix(#3468): handle visibility in tool update conflict checks Signed-off-by: Nithin Katta <Nithin.Katta@ibm.com> * fix(#3468): remove default value from visibility field in tool update schema Signed-off-by: Nithin Katta <Nithin.Katta@ibm.com> * test(#3468): update tool visibility conflict check tests to use mock_tool fixture Signed-off-by: Nithin Katta <Nithin.Katta@ibm.com> * fix(#3468): harden tool update to preserve omitted fields and check visibility conflicts - Change ToolUpdate.visibility default from "public" to None so omitting the field preserves the existing value instead of resetting to public. - Remove client-settable team_id/owner_email from ToolUpdate; derive ownership fields from the DB record for conflict checks. - Extract _check_tool_name_conflict() to handle public/team/private scoped uniqueness checks in a single reusable method. - Add conflict checking when visibility changes without a name change. - Fix custom_name_ref fallback to use existing custom_name when it diverges from name (instead of incorrectly using the new name). - Remove the else branch that wiped auth_type when auth was not provided. Closes #3468 Signed-off-by: Jonathan Springer <jps@s390x.com> * fix(#3468): add visibility value validation and warning log for incomplete conflict checks - Change ToolUpdate.visibility from Optional[str] to Optional[Literal["private", "team", "public"]] so Pydantic rejects invalid values at the schema boundary. - Add a warning log in _check_tool_name_conflict when a team/private visibility is missing the required team_id/owner_email, making skipped conflict checks observable. Signed-off-by: Jonathan Springer <jps@s390x.com> * fix(#3468): label private conflicts correctly in ToolNameConflictError ToolNameConflictError formatted private-scope conflicts as "Public" because the else branch caught both public and private visibility. This path is newly reachable since the PR adds private conflict detection. Add an explicit "private" branch and a test. Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> --------- Signed-off-by: Nithin Katta <Nithin.Katta@ibm.com> Signed-off-by: Jonathan Springer <jps@s390x.com> Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> Co-authored-by: Nithin Katta <Nithin.Katta@ibm.com> Co-authored-by: Jonathan Springer <jps@s390x.com> Co-authored-by: Mihai Criveti <crivetimihai@gmail.com> Signed-off-by: Yosief Eyob <yosiefogbazion@gmail.com>

KKNithin force-pushed the issue-3468-tool-update branch from a87cf56 to cf8deea Compare March 5, 2026 14:57

KKNithin closed this Mar 5, 2026

KKNithin reopened this Mar 5, 2026

KKNithin marked this pull request as ready for review March 5, 2026 15:28

KKNithin requested review from crivetimihai, kevalmahajan and madhav165 as code owners March 5, 2026 15:28

KKNithin changed the title ~~Issue 3468 tool update~~ fix(3468):Tool update resets visibility to public when field not provided Mar 5, 2026

jonpspri force-pushed the issue-3468-tool-update branch 2 times, most recently from e5bc346 to 2a64719 Compare March 5, 2026 21:52

jonpspri added the release-fix Critical bugfix required for the release label Mar 6, 2026

Nithin Katta and others added 5 commits March 7, 2026 00:22

fix(IBM#3468): handle visibility in tool update conflict checks

60aac51

Signed-off-by: Nithin Katta <Nithin.Katta@ibm.com>

fix(IBM#3468): remove default value from visibility field in tool upd…

63c3b9e

…ate schema Signed-off-by: Nithin Katta <Nithin.Katta@ibm.com>

test(IBM#3468): update tool visibility conflict check tests to use mo…

5367e34

…ck_tool fixture Signed-off-by: Nithin Katta <Nithin.Katta@ibm.com>

crivetimihai force-pushed the issue-3468-tool-update branch from 2a64719 to f62e6cb Compare March 7, 2026 00:50

crivetimihai previously approved these changes Mar 7, 2026

View reviewed changes

crivetimihai dismissed their stale review via 16f52d5 March 7, 2026 07:34

crivetimihai mentioned this pull request Mar 7, 2026

[BUG][API]: Tool update conflict checks have pre-existing gaps in name resolution and scope validation #3528

Open

crivetimihai self-assigned this Mar 7, 2026

crivetimihai added this to the Release 1.0.0-RC2 milestone Mar 7, 2026

crivetimihai merged commit fd31814 into IBM:main Mar 7, 2026
39 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(3468):Tool update resets visibility to public when field not provided#3503

fix(3468):Tool update resets visibility to public when field not provided#3503
crivetimihai merged 6 commits intoIBM:mainfrom
KKNithin:issue-3468-tool-update

KKNithin commented Mar 5, 2026

Uh oh!

jonpspri commented Mar 5, 2026

Uh oh!

crivetimihai commented Mar 7, 2026

Uh oh!

crivetimihai left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

KKNithin commented Mar 5, 2026

🔗 Related Issue

📝 Summary

🏷️ Type of Change

✅ Checklist

Uh oh!

jonpspri commented Mar 5, 2026

Uh oh!

crivetimihai commented Mar 7, 2026

Review Summary

Findings

Live E2E Test Results

Uh oh!

crivetimihai left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants